FFS-2110: Update flow to read/write indexing data to CbvApplicant #438

tdooner · 2025-02-06T18:39:34Z

Ticket

Resolves FFS-2110.

Changes

Rename CbvClient -> CbvApplicant
Write CbvApplicant fields directly from Caseworker form

Move all validations and logic for indexing fields onto CbvApplicant.
Use accepts_nested_attributes_for and fields_for to forward
inputs directly into that object
Add site_id to CbvApplicant since it will be a pain to go backwards
to either the CbvFlow or CbvFlowInvitation to get the site_id in all
of the places it's used.
Write all the fields to CbvFlowInvitation as well, just for
safekeeping until we drop them from there.

Test updates:

Update factory for CbvFlow and CbvFlowInvitation to also create a
cbv_applicant. Tests can override the fields of the applicant by
passing in cbv_applicant_attributes to the factory when creating
either the flow or the invitation.

Context for reviewers

Uhhh, this is a pretty big refactor. Planning to get some more eyes on this for smoke testing before merging.

TODO list (as of 2/6):

Acceptance testing

No acceptance testing needed
- This change will not affect the user experience (bugfix, dependency updates, etc.)
Acceptance testing prior to merge
- This change can be verified visually via screenshots attached below or by sending a link to a local development environment to the acceptance tester
- Acceptance testing should be done by design for visual changes, product for behavior/logic changes, or both for changes that impact both.
Acceptance testing after merge
- This change is hard to test locally, so we'll test it in the demo environment (deployed automatically after merge.)
- Make sure to notify the team once this PR is merged so we don't inadvertently deploy the unaccepted change to production. (e.g. :alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!)

* Move all validations and logic for indexing fields onto CbvApplicant. * Use `accepts_nested_attributes_for` and `fields_for` to forward <form> inputs directly into that object * Add `site_id` to CbvApplicant since it will be a pain to go backwards to either the CbvFlow or CbvFlowInvitation to get the site_id in all of the places it's used. * Write all the fields to CbvFlowInvitation as well, just for safekeeping until we drop them from there. Test updates: * Update factory for CbvFlow and CbvFlowInvitation to also create a cbv_applicant. Tests can override the fields of the applicant by passing in `cbv_applicant_attributes` to the factory when creating either the flow or the invitation.

app/app/controllers/api/invitations_controller.rb

app/app/controllers/caseworker/cbv_flow_invitations_controller.rb

app/app/models/cbv_applicant.rb

app/app/models/cbv_flow.rb

It's slightly nicer than having a setter for the attributes.

* Convert the `has_one` relationships into `has_many` * Add redaction logic

…ow-use-new-database * origin/main: FFS-2295: Help modal (#417)

Convert from an errors hash including nested errors (for example, for `cbv_applicant.first_name`) to match the API request structure (to `agency_partner_metadata.first_name`).

If `Rails.application.load_tasks` is called multiple times, than any invocation of a Rake task will actually happen multiple times. This is problematic for non-idempotent Rake tasks (like `users:promote_to_service_account`). Let's prevent this issue by just loading the tasks in the "rails_helper.rb" file so they're always available.

…ow-use-new-database * origin/main: Fix flaky rake tests by always loading Rake tasks once (#439)

If an identity job fails for a Pinwheel account, it will put a `nil` in this array, which will break our Pinwheel post-processing.

allthesignals

Amazing!! My only complaint is that we will continue to write those attributes to cbv_flow_invitation... should I make a ticket to remove that?

allthesignals · 2025-02-07T19:14:01Z

app/app/controllers/caseworker/cbv_flow_invitations_controller.rb

                        error_message: e.message)
      return redirect_to caseworker_dashboard_path(site_id: params[:site_id])
    end

    if @cbv_flow_invitation.errors.any?
+      @cbv_flow_invitation.errors.delete(:cbv_applicant)


Interesting. During an error scenario, because it's a separate model, we have to delete it. I'm surprised there isn't a way to configure this as part of validations? Could you say:

accepts_nested_attributes_for :asdf, :reject_if => :something_bad

This is only an issue because we iterate over the whole @cbv_flow_invitation.errors hash to generate the error alert message at the top of the page (e.g. "5 Errors: [list of errors]").

With cbv_applicant in the .errors hash, the count one-too-many and there is an error message for cbv_applicant that isn't meant to be user-facing.

allthesignals · 2025-02-07T19:18:46Z

app/app/controllers/help_controller.rb

-  rescue => ex
-    Rails.logger.error "Unable to track event (ApplicantViewedHelpTopic): #{ex}"
+    begin
+      event_logger.track("ApplicantViewedHelpTopic", request, {
+        topic: @help_topic,
+        cbv_applicant_id: cbv_flow&.cbv_applicant_id,
+        cbv_flow_id: session[:cbv_flow_id],
+        invitation_id: cbv_flow&.cbv_flow_invitation_id,
+        site_id: current_site&.id,
+        flow_started_seconds_ago: cbv_flow ? (Time.now - cbv_flow.created_at).to_i : nil,
+        language: I18n.locale
+      })
+    rescue => ex
+      Rails.logger.error "Unable to track event (ApplicantViewedHelpTopic): #{ex}"
+    end


Wait, I don't understand why. Is it bad form to use that approach to rescues (on the method)?

Rescue on a method is fine in general, but I didn't like it in this case because it might accidentally rescue exceptions for the other parts of the method that aren't related to event sending. It would make debugging the situation harder because we wouldn't get the actual exception reported to NewRelic.

allthesignals · 2025-02-07T19:22:33Z

app/app/models/cbv_applicant.rb

+  def parse_snap_application_date
+    raw_snap_application_date = @attributes["snap_application_date"]&.value_before_type_cast
+    return if raw_snap_application_date.is_a?(Date)
+
+    if raw_snap_application_date.is_a?(ActiveSupport::TimeWithZone) || raw_snap_application_date.is_a?(Time)
+      self.snap_application_date = raw_snap_application_date.to_date
+      # handle ISO 8601 date format, e.g. "2021-01-01" which is Ruby's default when querying a date field
+    elsif raw_snap_application_date.is_a?(String) && raw_snap_application_date.match?(/^\d{4}-\d{2}-\d{2}$/)
+      self.snap_application_date = Date.parse(raw_snap_application_date)
+    else
+      begin
+        new_date_format = Date.strptime(raw_snap_application_date.to_s, "%m/%d/%Y")
+        self.snap_application_date = new_date_format
+      rescue Date::Error => e
+        case site_id
+        when "ma"
+          error = :ma_invalid_date
+        when "nyc"
+          error = :nyc_invalid_date
+        else
+          error = :default_invalid_date
+        end
+        errors.add(:snap_application_date, error)
+      end
+    end
+  end
+
+  def ma_snap_application_date_not_in_future
+    if snap_application_date.present? && snap_application_date > Date.current
+      errors.add(:snap_application_date, :ma_invalid_date)
+    end
+  end
+
+  def ma_snap_application_date_not_more_than_1_year_ago
+    if snap_application_date.present? && snap_application_date < 1.year.ago.to_date
+      errors.add(:snap_application_date, :ma_invalid_date)
+    end
+  end
+
+  def nyc_snap_application_date_not_in_future
+    if snap_application_date.present? && snap_application_date > Date.current
+      errors.add(:snap_application_date, :nyc_invalid_date)
+    end
+  end
+
+  def nyc_snap_application_date_not_more_than_30_days_ago
+    if snap_application_date.present? && snap_application_date < 30.day.ago.to_date
+      errors.add(:snap_application_date, :nyc_invalid_date)
+    end
+  end
+
+  def format_case_number
+    return if case_number.blank?
+    case_number.upcase!
+    if case_number.length == 9
+      self.case_number = "000#{case_number}"
+    end
+  end
+
+  def paystubs_query_begins_at
+    PAYSTUB_REPORT_RANGE.before(snap_application_date)
+  end


I can't really tell from the diff but this is all copied oer from cbv_invitation, right?

Yeah, all were copied without modification. (I should have left a comment pointing that out, oops.)

allthesignals · 2025-02-07T19:23:43Z

app/app/models/cbv_applicant.rb

+  with_options(if: :nyc_site?) do
+    validates :case_number, format: { with: NYC_CASE_NUMBER_REGEX, message: :invalid_format }
+    validates :client_id_number, format: { with: NYC_CLIENT_ID_REGEX, message: :invalid_format }
+    validate :nyc_snap_application_date_not_more_than_30_days_ago
+    validate :nyc_snap_application_date_not_in_future
+  end


Just noting this is slightly different from how it was written in cbv_flow_invitation... seems like nicer syntax for the same thing?

Yeah, I tend to like using Rails's with_options to group together similar validation rules. The with_options helper will add whatever you pass it as an options hash to all the functions within. Helpful to make sure we don't forget to add the NYC conditional for any NYC validation rules.

allthesignals · 2025-02-07T19:26:36Z

app/app/services/data_retention_service.rb

+        next unless Time.now.after?(cbv_flow_invitation.expires_at + REDACT_UNUSED_INVITATIONS_AFTER)
+
+        cbv_flow_invitation.redact!
+        cbv_flow_invitation.cbv_applicant&.redact!


Aaaaah. Need a feature in redactable, like has_redactable_fields (hi: string), dependent: :redact or something

Yeah, was thinking about having the Redactable module redact associations, similar to how you had it. Would be good for a future change?

tdooner · 2025-02-10T18:33:52Z

My only complaint is that we will continue to write those attributes to cbv_flow_invitation... should I make a ticket to remove that?

@allthesignals Already created - https://jiraent.cms.gov/browse/FFS-2463 !

…ow-use-new-database * origin/main: FFS-2408: Rename site_id to client_agency_id (#446) FFS-2351: Change calculation of account_count to be correct in filed events (#445) Patching vulnerabilities | esbuild, net-imap (#450) Address vulnerability (#443) fix: upgrade postcss from 8.5.0 to 8.5.1 (#441) fix: upgrade sass from 1.83.2 to 1.83.4 (#440) 2401: Use different syntax (#437) updated tests for clarity fixed typo cleaned up based on PR comments removed trailing whitespace/rubocop fixes wrapped token creation for users.rake in a transaction, added test assertion to assure that the user api_access_tokens.count only changes by a factor of 1 updated vitest to v 3.0.5 address failed security scan rename vitest add github action fixed postcss build error added .vitest to git ignore updated package.json to use module setup project to work with vitest and debugging remove employer_search.spec comment out test write test scripts for pinwheel.js update apiservice to fetchInternalApiService for clarity refactored fetch into its own file refactored code to be a little more self evident remove outdated snapshots comment refactored api calls with tests added tests for trackUserAction api call move trackUserAction out of pinwheel into analytics file minor changes stub for employersearch test installed vitest

…ow-use-new-database * origin/main: FFS-2336: First sketch of multi-provider search (#442)

tdooner added 2 commits February 5, 2025 16:53

Rename CbvClient -> CbvApplicant

72f714b

tdooner marked this pull request as draft February 6, 2025 18:39